-
Notifications
You must be signed in to change notification settings - Fork 165
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat(deps): update puppeteer & typescript dependencies #862
Conversation
I will review it deeply later, but I see a lot of changes because you ran Why did you do it? |
updating eslint & all eslint configs caused few linting errors across multiple files. Added the "--fix" options to fix those minor errors and pass the lint stage. |
Hi @yanivfranco, upgrading technology stack is usually painful and I see you had quite a few breaking changes you tackled so thank you for your contribution. few things: Supporting the israeli-bank-scraper-coreI see that you removed file Node versionit works only with node v18. node v16 causes errors due to unsupported engines. I suggest to:
package-lock.json file sync issuesrunning
vulnerabilitiesI see Thanks again, it is an important PR regarding the lint, I"m ok with that, anyhow we have lint checks that prevent us from merging PRs with lint issues. @baruchiro please review it as well. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@@ -64,7 +64,7 @@ async function pageEvalAll<R>(page: Page | Frame, selector: string, | |||
result = await page.$$eval(selector, callback, ...args); | |||
} catch (e) { | |||
// TODO temporary workaround to [email protected] which breaks $$eval bevahvior until they will release a new version. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe we can open a new task to remove this part.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sure, this part is probably irrelevant now. should open an issue?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, and write a note that we should start working on it only after merging this PR.
@esakal Thanks, I am happy to contribute
@baruchiro pushed new iteration |
Hi everyone, we are in the middle of reviewing this, but please test it locally with your accounts to find any problem as soon as possible.
|
export function getPuppeteerConfig() { | ||
return { ...puppeteerConfig }; | ||
return { chromiumRevision: '1250580' }; // https://github.com/puppeteer/puppeteer/releases/tag/puppeteer-core-v22.5.0 | ||
|
||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@esakal I reviewed everything, I left the chromium version / scrapers-core to you.
"build:types": "tsc --emitDeclarationOnly", | ||
"build:puppeteer-config": "ncp src/puppeteer-config.json lib/puppeteer-config.json", | ||
"build:js": "babel src --out-dir lib --extensions \".ts\" --source-maps inline --verbose", | ||
"prebuild": "node utils/update-puppeteer-config.js", | ||
"postbuild": "rimraf lib/tests", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I finished my review, waiting for @esakal to review the |
Hi, As a side effect, I see a lot of warnings running the test: |
@gczobel Thanks, fixed |
Well done!! Looks good to me, I will talk with @esakal to check the |
i tested the following scrapers and all tests pass:
When running
After running diff --git a/package.json b/package.json
index 2e88e70..89fdd0a 100644
--- a/package.json
+++ b/package.json
@@ -63,7 +63,7 @@
"fs-extra": "^10.0.0",
"husky": "^8.0.3",
"jest": "^29.7.0",
- "jscodeshift": "^0.11.0",
+ "jscodeshift": "^0.16.1",
"minimist": "^1.2.5",
"ncp": "^2.0.0",
"rimraf": "^3.0.0",
@@ -80,7 +80,7 @@
"moment": "^2.22.2",
"moment-timezone": "^0.5.37",
"node-fetch": "^2.2.0",
- "puppeteer": "22.5.0",
+ "puppeteer": "^22.12.1",
"uuid": "^9.0.1"
},
"files": [ |
Hi, It happens since babel which uses browserlist throw
I didn't yet figure out how to satisfy browserlist, if you have any idea please share. |
@daniel-hauser Thanks for the response. I updated jscodeshift to the latest version, however there is an issue with the latest puppeteer version though. The binary still does not exists in the @esakal I will try to take a look. Can you maybe elaborate why jscodeshift is running in a child process and not as part of node current context? |
@esakal I got the |
@esakal @baruchiro Any update? |
I think the only thing left is the I tried to test it with the I hope I will test it in a new fork of Caspion soon. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@yanivfranco thank you! this will have a great impact on the project. Nice job with the code mod library
🎉 This PR is included in version 5.0.0 🎉 The release is available on GitHub release Your semantic-release bot 📦🚀 |
This PR is big. might be eligible for a new release.
But I think it is long overdue :)
before:

after:

In this PR:
This update had few collateral updates:
For the newer puppeteer version, there is no valid way to get the relevant used chromium revision from puppeteer. (see Programmatically access compatible Chromium revision number puppeteer/puppeteer#8203 (comment)). Regardless, in my opinion, this whole script is redundant and over engineered for this case. The chromium revision is static for the specific used puppeteer version, and can be retrieved easily from the release notes. This will need to be done each time puppeteer version is updated (which seems it didn't happen for 4 years so I think it won't make much of a difference)
Tested scrapers : discount, isracard & cal.